refactor(client)!: introduce ServiceParameters for extensions and include it in ClientCallContext#784
Conversation
…guglielmo-san/a2a-python into guglielmoc/refactor_base_client
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the client-side API by centralizing the management of service-level parameters, such as extensions, within a dedicated Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how client-side extensions are handled by introducing ServiceParameters within the ClientCallContext. This is a good architectural improvement that centralizes service-level parameters and cleans up method signatures across the client and transport layers.
However, there are a few significant points to consider:
- Breaking API Change: The
send_messagemethod signature has been changed to accept aSendMessageRequestobject instead of aMessageand other parameters. This is a major breaking change for users of the SDK and should be clearly communicated. - Interceptor Removal: The refactoring appears to have removed support for
ClientCallInterceptorfrom theJsonRpcTransport. This is another significant breaking change that is not mentioned in the PR description. The skipping of interceptor-related tests confirms this. This needs to be addressed, either by restoring the functionality or by explicitly documenting its removal. - Potential Bug: I've identified a potential logic reversal in
base_client.pyfor handling theblockingparameter in polling mode, which could lead to incorrect behavior.
I've left specific comments on the code with more details. Overall, the direction of the refactoring is positive, but these critical issues should be resolved before merging.
Note: Security Review did not run due to the size of the PR.
I am having trouble creating individual review comments. Click here to see my feedback.
src/a2a/client/transports/jsonrpc.py (436-457)
The _apply_interceptors method has been removed from this transport. This is a significant breaking change as it appears to remove support for client-side call interceptors for the JsonRpcTransport. This was not mentioned in the pull request description. If this removal was intentional, it should be documented. If it was accidental, it should be restored.
src/a2a/client/base_client.py (98-99)
There appears to be a logic reversal in how the blocking configuration is applied. The previous implementation set blocking=not self._config.polling, meaning if polling is enabled, the call is non-blocking. The new logic here appears to set blocking=True when polling is enabled, which is the opposite. This could be a bug.
src/a2a/client/transports/rest.py (366-392)
There is an inconsistency in how headers are handled between _execute_request and _send_stream_request. The _send_stream_request method correctly merges headers from self.httpx_client with request-specific headers. However, _execute_request only uses headers from the context (via _get_http_args) and does not include the base client headers from self.httpx_client.headers. This could lead to missing headers (e.g., A2A-Version) in non-streaming REST requests.
To ensure consistency, _execute_request should also merge the base client headers.
async def _execute_request(
self,
method: str,
target: str,
tenant: str,
payload: dict[str, Any] | None = None,
context: ClientCallContext | None = None,
) -> dict[str, Any]:
path = self._get_path(target, tenant)
http_kwargs = self._get_http_args(context)
payload = payload or {}
headers = dict(self.httpx_client.headers.items())
headers.update(http_kwargs.get('headers', {}))
timeout = http_kwargs.get('timeout', httpx.USE_CLIENT_DEFAULT)
json_payload = payload if method == 'POST' else None
params = payload if method != 'POST' else None
request = self.httpx_client.build_request(
method,
f'{self.url}{path}',
json=json_payload,
params=params,
headers=headers, # type: ignore[arg-type]
timeout=timeout, # type: ignore[arg-type]
)
return await self._send_request(request)tests/client/transports/test_jsonrpc_client.py (651-688)
This test contains a large block of commented-out code which seems to be from a previous implementation or debugging session. It would be best to remove this commented-out code to improve test readability.
…ts, convert `rpc_request.data` to dict, and adjust client blocking configuration logic.
…` keyword arguments and streamline `http_kwargs` passing.
…lper function used by REST and JSON-RPC transports.
guglielmo-san
changed the title
guglielmo-san
changed the title
…eter to ClientCallContext and update SendMessageRequest instantiation.
|
/gemini review |
…zing `ClientCallContext` for metadata and delete the now obsolete update script.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the client API by introducing ServiceParameters within ClientCallContext to handle extensions and other per-call parameters. This simplifies many method signatures and centralizes request configuration logic. The changes are applied consistently across transports and tests. However, I've identified a high-severity bug in the default blocking behavior for non-polling clients, a critical syntax error in compatibility code, along with a few minor suggestions for code cleanup.
Note: Security Review did not run due to the size of the PR.
I am having trouble creating individual review comments. Click here to see my feedback.
src/a2a/compat/v0_3/grpc_transport.py (88-91)
There appears to be a stray comma in the __init__ method signature, which is a syntax error.
def __init__(
self,
channel: Channel,
agent_card: a2a_pb2.AgentCard | None,
):
src/a2a/client/base_client.py (98-99)
The logic for setting the blocking flag based on self._config.polling appears to be incorrect. When self._config.polling is False, request.configuration.blocking should default to True, but with the current logic, it remains False. This breaks the expected behavior for non-polling clients that rely on blocking calls by default.
The original behavior was to default blocking to not self._config.polling. This new logic only correctly handles the case where polling is True.
tests/client/transports/test_jsonrpc_client.py (651-687)
These comments appear to be developer notes from during the refactoring. They could be removed to improve the readability of the test.
update_script.py (1-50)
This appears to be a one-off refactoring script. It should probably be removed from the pull request before merging.
…Transport` `__init__` method.
…port and simplify gRPC timeout retrieval.
2 participants
Description
This PR refactors the client API definitions to streamline extension handling and unify transport logic: